Skip to content

[interpreter] Drop IncrementalExecutor offsets to obtain LLJIT for Cling #589

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 23, 2025
Merged

[interpreter] Drop IncrementalExecutor offsets to obtain LLJIT for Cling #589

merged 1 commit into from
May 23, 2025

Conversation

@aaronj0
Copy link
Collaborator

@aaronj0 aaronj0 commented May 22, 2025

This patch drops pointer arithmetic used to obtain the llvm::orc::LLJIT from older Cling versions. This would make for safer usage of the cling::Interpreter and good to have for the 1.7.0 stable release that ROOT will move to.

@aaronj0 aaronj0 requested a review from vgvassilev May 22, 2025 16:31
@mcbarton
Copy link
Collaborator

mcbarton commented May 22, 2025 •
edited
Loading

@aaronj0 When I was looking at this in my PR to drop cling 1.0 support I was planning to replace every occurance of compat::getExecutionEngine in the code (given its now just a return statement). I just never got around to doing it. Maybe you can do it here?

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@codecov
Copy link

codecov bot commented May 22, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.43%. Comparing base (fd69d55) to head (3e80d55).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #589   +/-   ##
=======================================
  Coverage   76.43%   76.43%           
=======================================
  Files           9        9           
  Lines        3666     3666           
=======================================
  Hits         2802     2802           
  Misses        864      864
Files with missing lines Coverage Δ
lib/Interpreter/Compatibility.h 91.96% <ø> (ø)
Files with missing lines Coverage Δ
lib/Interpreter/Compatibility.h 91.96% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mcbarton
Copy link
Collaborator

@aaronj0 Given v1.7.0 just got released without this, do you mind me just doing this patch as part of my PR as part of my effort to drop cling 1.0 support here #581 ?

@aaronj0
Copy link
Collaborator Author

aaronj0 commented May 22, 2025

@aaronj0 Given v1.7.0 just got released without this, do you mind me just doing this patch as part of my PR as part of my effort to drop cling 1.0 support here #581 ?

I prefer to land this here as an atomic change...

@aaronj0 When I was looking at this in my PR to drop cling 1.0 support I was planning to replace every occurance of compat::getExecutionEngine in the code (given its not just a return statement). I just never got around to doing it. Maybe you can do it here?

hmm that doesn't make sense to me:

In a function that accepts a compat::Interpreter :

CppInterOp/lib/Interpreter/CppInterOp.cpp

Lines 3160 to 3162 in 392b754

bool InsertOrReplaceJitSymbol(compat::Interpreter& I,
const char* linker_mangled_name,
uint64_t address) {

That uses this API:

CppInterOp/lib/Interpreter/CppInterOp.cpp

Lines 3212 to 3217 in 392b754

// Let's inject it.
llvm::orc::SymbolMap InjectedSymbols;
auto& DL = compat::getExecutionEngine(I)->getDataLayout();
char GlobalPrefix = DL.getGlobalPrefix();
std::string tmp(linker_mangled_name);

we need the abstraction anyway since clang-repl requires extra handling:

CppInterOp/lib/Interpreter/Compatibility.h

Lines 342 to 345 in 392b754

inline llvm::orc::LLJIT* getExecutionEngine(clang::Interpreter& I) {
#if CLANG_VERSION_MAJOR >= 14
auto* engine = &llvm::cantFail(I.getExecutionEngine());
return const_cast<llvm::orc::LLJIT*>(engine);

Another motivation is also to keep the compat:: abstraction layer consistent, and perform operations defined under compat to compat::Interpreter

@aaronj0 aaronj0 merged commit 951775f into compiler-research:main May 23, 2025
78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Vipul-Cariappa Vipul-Cariappa Vipul-Cariappa approved these changes

@vgvassilev vgvassilev Awaiting requested review from vgvassilev

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@aaronj0 @mcbarton @Vipul-Cariappa